-
Notifications
You must be signed in to change notification settings - Fork 572
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support custom kwargs for model card in save_pretrained #2310
Conversation
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
The mixin currently have a set of very relevant model card metadata. For more customization, it's possible to overwrite the class Model(nn.Module, PytorchModelHubMixin):
...
def generate_model_card(self) -> ModelCard:
card = super(self).generate_model_card()
card.data.custom_key = "custom value"
return card I'd prefer not to add too many features to the main way of defining tags/library_name/... to avoid complexity and promote this flexible method instead. WDYT? |
The feature is inspired by this PR, as you may see I try to make it possible to pass a custom metrics dictionary and dataset name to be able to generate cards like this. model.save_pretrained('./my_model', metrics={'accuracy': 0.95}, dataset='my_dataset') You might want to pass arguments not at the time of model class creation, but at the time of model saving, for example when the model is trained. I overridden the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the clarification and sorry for the confusion on my side. You are totally right, this makes sense to implement. I just reviewed your code and the integration looks good 👍
Could you add a small test in test_hub_mixin_pytorch.py
with a new class that overwrites generate_model_card
+ pass data in save_pretrained
and check you have the expected result. Thanks in advance!
(small nit about the integration you've mentioned: it would be good to use super(self).generate_model_card()
in the integration and define model card template/library_name/tags/etc. in the class inheritance. This way models will be tagged as model_hub_mixin
and pytorch_model_hub_mixin
on the Hub which is important for us to track integrations and make sure to not break anything in the future if we update this Mixin.)
@Wauplin thank you for the feedback! I will update the integration, would be cool to include this PR changes, to get rid of private attributes :)
Done! Please, have a look! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amazing! Thanks for the prompt update :) Test is clean, CI is green, let's merge 🎉
Thanks for the quick review and merge 🤗 |
* Support custom kwargs for model card in save_pretrained * Fix failing test * Fix test for pytorch mixin * Add test for model_card_kwargs * Fix style
Hi!
There is an option to customize the model card for
Mixin
s, it would be great to support custom kwargs for a card also.Here is an example:
Generated readme:
Let me know what you think about this feature! If it's OK, I can add the necessary tests.